Remove java.util.Date #2373
Remove java.util.Date #2373
Conversation
jianglai
force-pushed
the
java-util-date
branch
from
8aa48af
to
517136a
Compare
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jianglai)
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status:
jianglai
force-pushed
the
java-util-date
branch
from
517136a
to
c845f74
Compare
config/presubmits.py
Outdated
| "java", | ||
| {"/node_modules/", "JpaTransactionManagerImpl.java"}, | ||
| ): | ||
| "Do not use java.util.Date. Use classes in Java.date package instead.", |
There was a problem hiding this comment.
What is the Java.date package? That doesn't seem correct to me.
| @@ -162,7 +158,7 @@ public ImmutableSet<CertificateViolation> checkCertificate(X509Certificate certi | |||
| ImmutableSet.Builder<CertificateViolation> violations = new ImmutableSet.Builder<>(); | |||
|
|
|||
| // Check if currently in validity period | |||
| Date now = clock.nowUtc().toDate(); | |||
| DateTime now = clock.nowUtc(); | |||
There was a problem hiding this comment.
This is a functionality change? Are you sure this is even correct?
What's the motivation for removing Date everywhere anyway? Sometimes you do legitimately need to process just a date, not a date and an attached time.
| description = | ||
| "The next date that the bulk pricing package should be billed for its annual fee") | ||
| Date nextBillingDate; | ||
| DateTime nextBillingDate; |
There was a problem hiding this comment.
This seems like a bad change to me? Why are we going to make people specify an entire date time on the command line when the only thing that should be passed in is a date?
| issuerDnName, serialNumber, from, to, subjectDnName, keyPair.getPublic()); | ||
| issuerDnName, | ||
| serialNumber, | ||
| from.toDate(), |
There was a problem hiding this comment.
This is literally still using a java.util.Date then, only it's purposely being packaged into a DateTime, and then stripped out of it, for no reason!
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
config/presubmits.py line 192 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
What is the
Java.datepackage? That doesn't seem correct to me.
My bad. Should be java.time.
core/src/main/java/google/registry/flows/certs/CertificateChecker.java line 161 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
This is a functionality change? Are you sure this is even correct?
What's the motivation for removing
Dateeverywhere anyway? Sometimes you do legitimately need to process just a date, not a date and an attached time.
java.util.Date is not just a date. It's an instant in time with millisecond precision. There are many problems with it(no timezone support, no calendar support, etc) and most of its methods are deprecated for decades (since JDK 1.1). That's the whole reason why people prefer Joda Time instead of Java.util.Date.
core/src/main/java/google/registry/tools/CreateOrUpdateBulkPricingPackageCommand.java line 63 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
This seems like a bad change to me? Why are we going to make people specify an entire date time on the command line when the only thing that should be passed in is a date?
Even with Date people are going to have to specify the time (see the other comment on Data being an instant in time). I don't know how jcommander handlers Date conversions, it may work if you don't specify one and it gives you some default time, but without timezone support that time is also ambiguous. We have been using DateTime in all other commands.
In general java.util.Date has such bad APIs that everyone should run from it unless they absolutely have to (like when their dependencies still expect it). Even then one should defer the conversion to Date until the last minute, preferably manipulating Joda Time or java.time until they call toDate() to convert it and pass it to the dependencies.
networking/src/test/java/google/registry/networking/handler/SslInitializerTestUtils.java line 85 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
This is literally still using a
java.util.Datethen, only it's purposely being packaged into a DateTime, and then stripped out of it, for no reason!
We have no control over how our dependencies' interfaces are defined, but we can avoid directly interacting with the deprecated java.util.date API as much as possible.
jianglai
force-pushed
the
java-util-date
branch
from
c845f74
to
3b37db9
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)
core/src/main/java/google/registry/flows/certs/CertificateChecker.java line 161 at r2 (raw file):
Previously, jianglai (Lai Jiang) wrote…
java.util.Dateis not just a date. It's an instant in time with millisecond precision. There are many problems with it(no timezone support, no calendar support, etc) and most of its methods are deprecated for decades (since JDK 1.1). That's the whole reason why people prefer Joda Time instead of Java.util.Date.
How about https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/LocalDate.html then? That looks to do what we need it to do here, but without that baggage.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
core/src/main/java/google/registry/flows/certs/CertificateChecker.java line 161 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
How about https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/LocalDate.html then? That looks to do what we need it to do here, but without that baggage.
In this case, we definitely want both a date and a time (as it pertains to checking the expiration date of certificates or CRLs).
In general, the recommendation is to use java.time for greenfield projects and joda.time for existing project. We have a mixture of both at the moment, though I think it might be a good idea to pick a lane and stick to it to avoid unnecessary conversions. Joda actually recommends actively migrating to java.time, but we have a lot of joda.time and the migration might not be worth the effort.
jianglai
force-pushed
the
java-util-date
branch
from
3b37db9
to
d1f0720
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
jianglai
force-pushed
the
java-util-date
branch
2 times, most recently
from
a1e0710
to
d211ef2
Compare
There is one remaining instance in JpaTransactionManagerImpl that cannot be removed because DetachingTypedQuery is implementing TypedQuery, which has a method that expectred java.util.Date.
jianglai
force-pushed
the
java-util-date
branch
from
d211ef2
to
2f2b892
Compare
There is one remaining instance in JpaTransactionManagerImpl that cannot
be removed because DetachingTypedQuery is implementing TypedQuery, which has
a method that expectred java.util.Date.
This change is